Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to 64-bit ints. #1357

Merged
merged 1 commit into from
Jan 19, 2024
Merged

Switch to 64-bit ints. #1357

merged 1 commit into from
Jan 19, 2024

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Jan 10, 2024

Syncing features with godotengine/godot#86730, but since no GDExtension public API is changed, it should be compatible both ways.

@bruvzg bruvzg added enhancement This is an enhancement on the current functionality topic:gdextension This relates to the new Godot 4 extension implementation labels Jan 10, 2024
@bruvzg bruvzg added this to the 4.3 milestone Jan 10, 2024
@AThousandShips AThousandShips added the waiting for Godot This issue needs a Godot Engine improvement to be solved label Jan 10, 2024
@AThousandShips
Copy link
Member

Should fix:

@AThousandShips AThousandShips removed the waiting for Godot This issue needs a Godot Engine improvement to be solved label Jan 10, 2024
@bruvzg bruvzg marked this pull request as ready for review January 10, 2024 21:41
@bruvzg bruvzg requested a review from a team as a code owner January 10, 2024 21:41
Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

This looks great to me 🚀

I diff'ed the cowdata.hpp and vector.hpp from this PR with the cowdata.h and vector.h from PR godotengine/godot#86730, and they only had minor expected differences. Reading through all the rest, it all looks good.

I haven't tested it manually, but the fact that it's passing the automated tests is a very good sign.

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 19, 2024

I know we can merge this before Godot, however, I'm holding off on merging for a bit, just in case there's any further changes to cowdata.h and vector.h on the Godot PR that we'll want to sync over here. When that one seems like it's finished and about to be merged, we can merge here.

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 19, 2024

PR godotengine/godot#86730 was just merged. I diffed the vector.h and cowdata.h with godot-cpp's vector.hpp and cowdata.hpp from this PR again, and didn't see any further changes that need to be sync'd. So, I think this one is good to merge now!

@dsnopek dsnopek merged commit 0145e90 into godotengine:master Jan 19, 2024
12 checks passed
@dsnopek
Copy link
Collaborator

dsnopek commented Jan 19, 2024

Thanks!

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 22, 2024

Cherry-picking to 4.2 in #1372

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 22, 2024

Cherry-picking to 4.1 in #1373

@AThousandShips
Copy link
Member

Assuming this doesn't actually depend on the upstream changes? As they aren't cherry picked, we might want to be careful here as it does have some side effects that might be harmful:

I'd hold off cherry picking this until we have worked things out

@bruvzg
Copy link
Member Author

bruvzg commented Jan 22, 2024

It doesn't depend on upstream change, but won't fix any of the issues without them, so is not useful on its own.

@@ -52,6 +52,8 @@ class VMap;
template <class T>
class CharStringT;

SAFE_NUMERIC_TYPE_PUN_GUARANTEES(uint64_t)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specifically breaks on some platforms, still being investigated

@YuriSizov
Copy link
Contributor

Should fix:

Did it fix this issue btw?

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 24, 2024

Should fix:

Did it fix this issue btw?

Yes it did! I've just closed that issue.

@dsnopek
Copy link
Collaborator

dsnopek commented Mar 11, 2024

Cherry-picked for 4.2 in PR #1410

@dsnopek
Copy link
Collaborator

dsnopek commented Mar 11, 2024

Cherry-picked for 4.1 in PR #1411

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality topic:gdextension This relates to the new Godot 4 extension implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants